Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(compiler): reexport less symbols in .ngfactory.ts files #19884

Closed
wants to merge 1 commit into from
Closed

fix(compiler): reexport less symbols in .ngfactory.ts files #19884

wants to merge 1 commit into from

Conversation

tbosch
Copy link
Contributor

@tbosch tbosch commented Oct 24, 2017 •
edited

  • don't reexport symbols that the user already reexported
  • never reexport symbols that are part of arguments of non simple function calls

Fixes #19883

@tbosch tbosch added state: WIP area: core Issues related to the framework runtime target: major This PR is targeted for the next major release labels Oct 24, 2017
@mary-poppins
Copy link

You can preview 28ed2fd at https://pr19884-28ed2fd.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 6a81fa0 at https://pr19884-6a81fa0.ngbuilds.io/.

@tbosch tbosch removed the state: WIP label Oct 27, 2017
@tbosch tbosch requested a review from chuckjaz October 27, 2017 20:52
@tbosch tbosch changed the title wip - use reexports in the code. fix(compiler): reexport less symbols in .ngfactory.ts files Oct 27, 2017
@mary-poppins
Copy link

You can preview 4fc578b at https://pr19884-4fc578b.ngbuilds.io/.

chuckjaz
chuckjaz approved these changes Oct 27, 2017
View reviewed changes
packages/compiler/src/aot/summary_serializer.ts Outdated
@@ -126,9 +127,22 @@ class ToJsonSerializer extends ValueTransformer {
}
});
metadata = clone;
} else if (isCall(metadata)) {
if (!isFunctionCall(metadata) && !isMethodCallOnVariable(metadata)) {
// Don't store complex calls as we won't be able to simplify them anyways lateron.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: later on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

packages/compiler/src/aot/summary_serializer.ts Outdated
} else if (isCall(metadata)) {
if (!isFunctionCall(metadata) && !isMethodCallOnVariable(metadata)) {
// Don't store complex calls as we won't be able to simplify them anyways lateron.
metadata = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning undefined you should replace it with an {__symbolic: 'error }` instead as this might cause a silent failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

packages/compiler/src/aot/summary_serializer.ts Outdated
}
} else if (isContructorCall(metadata)) {
// don't store `new` calls
metadata = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mary-poppins
Copy link

You can preview 3b9ee40 at https://pr19884-3b9ee40.ngbuilds.io/.

chuckjaz
chuckjaz approved these changes Oct 30, 2017
View reviewed changes
packages/compiler/src/aot/util.ts Outdated

export function isLoweredSymbol(name: string) {
return /\u0275\d+/.test(name);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to a const

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@mary-poppins
Copy link

You can preview b5b67ec at https://pr19884-b5b67ec.ngbuilds.io/.

@tbosch
fix(compiler): reexport less symbols in .ngfactory.ts files
3bbecb0 
* don't reexport symbols that the user already reexported
* never reexport symbols that are part of arguments of non simple function calls

Fixes angular#19883
@mary-poppins
Copy link

You can preview 3bbecb0 at https://pr19884-3bbecb0.ngbuilds.io/.

@tbosch
Copy link
Contributor Author

tbosch commented Oct 30, 2017

Note: this is green on global TAP.

@tbosch tbosch added the action: merge The PR is ready for merge by the caretaker label Oct 30, 2017
@tbosch tbosch added this to the v5.0.0 milestone Oct 30, 2017
matsko pushed a commit that referenced this pull request Oct 31, 2017
@tbosch @matsko
fix(compiler): reexport less symbols in .ngfactory.ts files (#19884)
e3a16ed 
* don't reexport symbols that the user already reexported
* never reexport symbols that are part of arguments of non simple function calls

Fixes #19883

PR Close #19884
@matsko matsko closed this in 420852e Oct 31, 2017
@tbosch tbosch deleted the less_reexports branch November 1, 2017 16:16
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chuckjaz chuckjaz chuckjaz approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
v5.0.0
Development

Successfully merging this pull request may close these issues.

Reexport less symbols in .ngfactory files (b/67995591)
4 participants
@tbosch @mary-poppins @chuckjaz @googlebot